I think I'd split the part specific to Touch into a separate PR. There may be questions about how we want to approach this specific variant that are unrelated to supporting libhybris

Tbf I've waited pretty long on the previous PR that has been sitting for a while, which used to be the libhybris-only PR. But if more people want to have it split up, then sure.

I would split it. The s-c bits should be fairly easy to land, but folks may be asking questions about this one and there's no point in holding back the rest of the PR.

Done.

I think you can reuse the buffer since snprintf always adds terminating \0 and simply interleave snprintf/fs operation sequence

Also, please use PATH_MAX where appropriate:

Addressed.

looks like a copy of sc_populate_libgl_with_hostfs_symlinks, could we make the former non-static and share the code?

Sure, will do

Taken care of.

again, can we share sc_mkdir_and_mount_and_glob_files from nvidia support code?

Yup, noted

Done.

This feels misplaced, neither snap-confine nor any part of the snap execution chain delivered by snapd sets any of the environment variables related to mesa/vulkan/egl. Most likely this should be part of to https://github.com/snapcore/snapcraft-desktop-integration

IMO having snap-confine set up the environment by itself allows for:

• Expected environment for any Snap
• Less ways of the Snap screwing up something in the setup (because it misses -desktop-integration or does things itself)
• Have existing Snaps working as-is.

I cannot expect upstream app developers to care or have the knowledge to setup libhybris appropriately, which is why I put it there.

EDIT: Also I would argue that the libhybris enablement is for more than just GL drivers, it actually allows any appropriately wrapped bionic-library to function inside of a Snap.

Hm I think I'd rather place it somewhere around here: https://github.com/snapcore/snapd/blob/68fe7eb9f7496ec54f2b3373a5c8d1993b9eb822/cmd/snap-exec/main.go#L211-L213 there's a precedent for it already for Cups

cc @zyga ?

Taken care of.

How is the library resolved at runtime, the directory will be from the perspective of a pivot_root base snap which (I suspect?) does not carry it.

This particular library resides on the host, which then gets picked up by snap-confine AFAICT.

In general thread-locals in a shared library are taken care of after initialization by ld, before the actual executable is run. Think calling constructors of a static variable, it's similar in style to how ld treats thread-locals.

Just for the record, host files do not appear at that location. What you see here comes from the base snap.

I am surprised then that snap-confine correctly stops complaining about not being able to map that library.

If you're setting LD_PRELOAD in your environment, than the denial from s-c should be raised regardless of this interface, as s-c is running under a different profile. Now, setting LD_PRELOAD in the environment will not have an effect of the snap as IIRC LD_PRELOAD isn't carried over to the snap's environment. Now, if OTOH is being set within the snap, then yes, if you allow it explicitly in the file like here, the denial will go away even if the file doesn't exist. Can you paste the denial you saw?

After giving it some time, I've come to the conclusion that we might still want to pass through the preload hack to the confined environment in case it's running on Touch specifically. Would that be feasible from your perspective?

ld.so will strip out LD_PRELOAD before it switches execution to snap-confine, i.e. snap-confine will not even see that environment variable, so there's no chance of passing it to the snap. Perhaps you can load it from /etc/environment when executing in the snap, but it would not be automatic.

I was more thinking of measuring whether it's running on Touch via other means (checking for existance of a Halium-only file) and recreating the environment variable. It might sound hacky at first but there really seems to be no easy other way.

EDIT: I suppose that needs to happen in the desktop-helpers then? In that case I would reduce this MR by just allowing mapping the file from the hostfs and do the rest in the launchers. Or how about a SNAP_LD_PRELOAD which allows snapd to dictate the environment variable?

I presume you've ran some apps with this change built locally. Were they able to start and function properly?

No I couldn't as my assumption about Halium 9 vs 10 was wrong. I thought one of my test devices was susceptible to the issue, whereas it was just Halium 10 MediaTek devices specifically (which seem to have TLS slots hardcoded in their bionic-based GL libs).

Curious, where is properties coming from? Is this some Android-specific interface? What's inside?

The __properties__ part contains SELinux info of process labels allowed to access Android properties through getprop and setprop. It's more of a void on UT though since functionality tied to SELinux is stubbed out to not conflict with AppArmor.

Does it need rw access or just read?

Potentially only needs read-access, since that is set up in the Halium container. But I'll have to check.

Addressed

nitpick: this does't appear to be a tmpfs so this error message is misleading

Handled in d5a85d6

It seems like this should be removed given the prior discussions that it appears unneeded.

it is not needed for most devices, but rather for a specific series of devices from a specific Android generation: Android 10 Mediatek, which I don't own, so I cannot figure out a solution by myself.

I am fine with the other changes landing but realistically we will need a solution for Mediatek devices with Android 10 too.

Done. Handled in d5a85d6

nitpick: would this be easier to read if it was written as something like the following (note untested):

const char *paths[] = {"/dev/binderfs/binder", "/dev/binderfs/hwbinder", "/dev/binder", "/dev/hwbinder"};
for (int i = 0; i < sizeof(paths)/sizeof(paths[0]); i++) {
    struct stat sbuf;
    if (stat(paths[i], &sbuf) == 0) {
        sc_device_cgroup_allow(cgroup, S_IFCHR, major(sbuf.st_rdev),
				       minor(sbuf.st_rdev));
    }
}

Thanks, based it on your template. Handled in d5a85d6

Again this should be removed since it appears unnecessary

Please check and address my previous comment above, we still need a solution for Mediatek Android 10 devices and I doubt waiting for much longer is a good idea.

Alright, short-term I am fine without supporting MTK-10 devices for now, but I would really like to have this handled later.

Handled in d5a85d6

Should this be checked and should s-c die if it does not work? If it is legitimately allowed to fail, as indicated by the (void) up front, can you add a comment just above
to explain why.

Can take a deeper look later after $DAYJOB

Handled in d5a85d6

Since this runs everywhere, what happens if someone installs, say, anbox and gets /dev/binder. Now all snaps will allow access to the binder. I don't have an immediate idea on how to make this better without some host-level hint.

The host level hint can again be a check for existance of /system/build.prop, which under Halium distributions always has to exist.

Handled in d5a85d6